Skip to content

Example using enzyme-auto-sparsity #69

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Apr 25, 2025
Merged

Conversation

nkoukpaizan
Copy link
Collaborator

This example uses enzyme-auto-sparsity to compute Jacobians directly in a sparse format.

@nkoukpaizan nkoukpaizan added development Features/Tools related to development of GridKit, rather than use as a library. automatic differentiation labels Mar 6, 2025
@nkoukpaizan nkoukpaizan self-assigned this Mar 6, 2025
@nkoukpaizan nkoukpaizan force-pushed the nicholson/enzyme-sparse branch 2 times, most recently from 931d7df to 2d1a06f Compare March 6, 2025 22:08
@nkoukpaizan nkoukpaizan marked this pull request as draft March 7, 2025 14:42
template <typename T>
__attribute__((always_inline)) static void sparse_store(T val, int64_t idx, size_t i, std::vector<Triple<T>>& triplets)
{
if (val == 0.0)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I worry that this is still computing 0s and looping over them at runtime, rather than computing sparsity at compile time

Copy link
Collaborator Author

@nkoukpaizan nkoukpaizan Mar 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC from when @wsmoses showed us the LLVM IR for a simpler example, it showed the structural zeros being optimized out. I've messed with it since (https://fwd.gymni.ch/U2yB6g) and I don't understand LLVM enough to point it out, though.

It's also the case that non-structural zeros are stored at runtime, which would not be the case if just checking the values at run time. In this example, I purposefully chose the first value of the input to be 0 to check that the 0 derivative gets returned.

Copy link
Collaborator

@alexander-novo alexander-novo Mar 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried removing the debug statements, and I can see that the structural zeroes are removed from consideration, which is nice. Check around line 170 in the output - you can see that it calculates the derivative input[i] * i * 2.0 and then returns that value with the index (i,i).

It looks like this is due to the -enzyme-auto-sparsity=1 flag? This flag seems a bit volatile... for example I can get this example (which represents more what the models in GridKit actually look like) to work with -enzyme-auto-sparsity=0, but it does do the runtime checks for non-structural zeroes (it does still automatically elide the structural zeroes, though). Changing the flag to -enzyme-auto-sparsity=1 causes Enzyme to fail to compile. It seems this flag has a fairly fragile dependence on functions which have a size parameter passed in and loop until that parameter, so we can reintroduce the N variable, which causes it to compile, but now there are many runtime checks on the N variable...

This may be worth submitting an issue to Enzyme about, since it seems like a fairly reasonable example?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like it's because of loop unrolling. If you disable loop unrolling it works fine. Good thing to know for the future!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there further actions needed from me here for this PR?

@shakedregev
Copy link
Collaborator

shakedregev commented Mar 7, 2025

The first thing I'd check is the failing test. Going over the rest.
Edit: just saw this is a draft.
Make sure the documentation is Doxygen compatible.

{
assert(0 && "should never load");
}

Copy link
Collaborator

@shakedregev shakedregev Mar 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this function do? Except for the fact that one should not call it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a "dummy" function required for calling __enzyme_todense, but on an output variable, so it should never be read from.

@nkoukpaizan nkoukpaizan force-pushed the nicholson/enzyme-sparse branch from 1c1822c to 1a0b3d2 Compare April 9, 2025 15:56
@nkoukpaizan nkoukpaizan marked this pull request as ready for review April 14, 2025 13:13
@nkoukpaizan
Copy link
Collaborator Author

I fixed the CI pipeline.

@nkoukpaizan nkoukpaizan force-pushed the nicholson/enzyme-sparse branch from 1a0b3d2 to 1fa564d Compare April 15, 2025 20:14
@nkoukpaizan nkoukpaizan force-pushed the nicholson/enzyme-sparse branch from 1fa564d to 9b417b1 Compare April 22, 2025 00:11
@nkoukpaizan
Copy link
Collaborator Author

The first thing I'd check is the failing test. Going over the rest. Edit: just saw this is a draft. Make sure the documentation is Doxygen compatible.

  • CI pipeline is fixes.
  • Added extra '/' on comments in 9b417b1. Should the requirement for Doxygen-compatible documentation be in the style guidelines?

@alexander-novo
Copy link
Collaborator

The first thing I'd check is the failing test. Going over the rest. Edit: just saw this is a draft. Make sure the documentation is Doxygen compatible.

* CI pipeline is fixes.

* Added extra '/' on comments in [9b417b1](https://github.com/ORNL/GridKit/pull/69/commits/9b417b13fb54181a01da874e3d203997a41de1a4). Should the requirement for Doxygen-compatible documentation be in the style guidelines?

I think it should, but in the meantime, it is mentioned in the new PR template, at least.

Copy link
Collaborator

@pelesh pelesh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The FindEnzyme.cmake is looking for llvm-link and opt tools in Enzyme_LLVM_BINARY_DIR, which is I believe incorrect location. This path is to the location of the LLVM installation, e.g. llvm-* directory. I think what we are looking for is the bin directory inside Enzyme_LLVM_BINARY_DIR, which LLVM stores in a variable LLVM_TOOLS_BINARY_DIR.

An easy fix is to do something like this:

find_program(GRIDKIT_LLVM_LINK llvm-link
             PATHS ${Enzyme_LLVM_BINARY_DIR}
             PATH_SUFFIXES
             bin
             REQUIRED)

Similar for the opt tool.

Otherwise good to merge. Builds correctly and tests pass.

@nkoukpaizan nkoukpaizan merged commit cd6b574 into develop Apr 25, 2025
3 checks passed
@pelesh pelesh deleted the nicholson/enzyme-sparse branch June 10, 2025 18:01
WiktoriaZielinskaORNL pushed a commit that referenced this pull request Jul 23, 2025
* Simple test with enzyme-auto-sparsity. Needs cleanup.

* Ran clang-format on EnzymeSparse.

* Add some comments in EnzymeSparse.

* Reference sparse Jacobian for EnzymeSparse example.

* Comparison between sparse matrices.

* Find Enzyme Clang plugin and require enzyme@main in CI to get the sparsity detection.

* dinput -> output.

* Try [email protected] in CI.

* Update CI to install additional LLVM libs needed for Enzyme to build ClangEnzyme*.so.

* Extra / on new comments added.

* Location to look for llvm-link and opt.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automatic differentiation development Features/Tools related to development of GridKit, rather than use as a library.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants